Skip to content

feat(integration): ask whether to review pull requests when connecting GitHub - #87

Merged
justinhelmer merged 1 commit into
mainfrom
feat/github-connect-pr-reviews
Sep 4, 2026
Merged

feat(integration): ask whether to review pull requests when connecting GitHub#87
justinhelmer merged 1 commit into
mainfrom
feat/github-connect-pr-reviews

Conversation

@justinhelmer

@justinhelmer justinhelmer commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

polylane integration connect --type github now asks one question before it opens the browser: review pull requests for production impact on the repositories this connection brings in (default yes). Teams that do not want those reviews say so once, in the terminal, instead of turning them off repository by repository afterwards.

What & why

Every repository a GitHub connection brings in is reviewed for production impact as soon as it syncs. The server side (coreplanelabs/nominal#2087, on UAT) records a connect-time answer on the integration and stamps it onto each repository the sync creates. This PR is the CLI surface for that answer. Validation ledger across both PRs: coreplanelabs/nominal#2089.

The question is asked exactly where the CLI already hands off to the browser, so the installer needs no change: install.sh runs this command interactively and the question appears in the install flow as-is, between the script's own Connect GitHub? [Y/n] and Connect Slack? [Y/n]. It is asked in the same [Y/n] shape as those lines (Enter = yes, only an n answer opts out), not as a clack confirm, so the installer reads as one flow. Two flags answer without the prompt for agents and scripts. The answer rides the existing /cli/connect URL as pr_reviews, and the console records it before the repository sync starts; the browser never asks a second time.

Validated end to end on UAT before release through the onboarding lab with this branch's bundle (nominal#2120 adds the knob): runs 260904001733e7vy (answer no → pr_reviews=off, integration opted out) and 260904001941emf0 (default yes → pr_reviews=on).

Tour

1. Deciding whether to ask

--no-pr-reviews and --pr-reviews answer without the prompt; an interactive run asks; a non-interactive run without a flag sends nothing, so the server keeps its default and records no answer.

Look for: opt-out wins if both flags are passed; undefined (not asked) and 'on' (asked, kept) are deliberately different values.

// The one question the GitHub connect asks: review pull requests for production impact on
// the repositories this connection brings in. A flag answers it without the prompt; an
// interactive run asks (default yes); a non-interactive run without a flag sends nothing, so
// the server keeps its default and records no answer.
export type PrReviewsChoice = 'on' | 'off';
export function prReviewsChoiceFromFlags(
args: Record<string, unknown>,
interactive: boolean
): PrReviewsChoice | 'ask' | undefined {
if (getArgBoolean(args, 'noPrReviews') === true) return 'off';
if (getArgBoolean(args, 'prReviews') === true) return 'on';
return interactive ? 'ask' : undefined;
}

2. The flag guard and the URL

prReviewsFlagsGiven lets a non-GitHub type warn that the flags are ignored (a warning, not an error, because the type can be picked interactively after the flag was typed). githubConnectUrl appends pr_reviews only when there is an answer; cliConnectUrl always carries ?flow=…&workspace=…, so the bare & is safe.

// The flags only mean something for GitHub; on any other type they are ignored with a warning
// rather than an error, because the type may have been picked interactively after the flag.
export function prReviewsFlagsGiven(args: Record<string, unknown>): boolean {
return getArgBoolean(args, 'noPrReviews') === true || getArgBoolean(args, 'prReviews') === true;
}
// The console's /cli/connect page carries the answer across the GitHub install round-trip
// and hands it to the API, which records it on the integration before the repository sync.
export function githubConnectUrl(config: Config, workspaceId: string, prReviews: PrReviewsChoice | undefined): string {
const url = cliConnectUrl(config, 'github', workspaceId);
return prReviews ? `${url}&pr_reviews=${prReviews}` : url;
}

3. The installer's [Y/n] shape

parseYesNo is install.sh's ask_yn in TypeScript: Enter keeps the default, only an answer starting with n (or y, for a default-no question) flips it, anything else keeps the default. promptYesNoOrBack prints <question> [Y/n] and reads one line from stdin, which run_tty in the installer hands the terminal to; a closed stream is BACK.

cli/src/utils/prompt.ts

Lines 128 to 151 in 04b8c18

export function parseYesNo(answer: string, defaultYes: boolean): boolean {
const first = answer.trim().charAt(0).toLowerCase();
if (first === 'n') return false;
if (first === 'y') return true;
return defaultYes;
}
export async function promptYesNoOrBack(
ctx: PromptContext,
message: string,
defaultYes = true
): Promise<boolean | typeof BACK> {
ensureInteractive(ctx, message);
const rl = createInterface({ input: process.stdin, output: process.stdout });
try {
const answer = await new Promise<string | null>((resolve) => {
rl.once('close', () => resolve(null));
rl.question(`${message} ${defaultYes ? '[Y/n]' : '[y/N]'} `, resolve);
});
return answer === null ? BACK : parseYesNo(answer, defaultYes);
} finally {
rl.close();
}
}

4. The question

One plain sentence saying what a review is and where to change it later, then the [Y/n] line. Cancel is BACK, which restarts the type picker like every other step.

// Asked in the installer's own `[Y/n]` shape, not a clack confirm: inside install.sh this line sits
// between the script's "Connect GitHub? [Y/n]" and "Connect Slack? [Y/n]" and must read like them.
async function askPrReviews(config: Config): Promise<PrReviewsChoice | typeof BACK> {
process.stdout.write(
'Polylane comments a pass or fail production-impact verdict on every pull request in the repositories you connect; change it per repository any time in the console.\n'
);
const keep = await promptYesNoOrBack(
{ nonInteractive: config.nonInteractive },
'Review pull requests for production impact?',
true
);
if (keep === BACK) return BACK;
return keep ? 'on' : 'off';
}

5. Where it runs in the GitHub connect

After the already-connected short-circuit (an existing connection is never re-asked) and before the browser opens. canWaitForBrowser is the interactivity test, so --output json, --dry-run, and non-interactive runs never see the prompt. A non-GitHub type with either flag prints the one-line warning and carries on.

const check = canWaitForBrowser(config) && baseline ? baseline.check : null;
let url = cliConnectUrl(config, type, workspaceId);
if (type !== 'github' && prReviewsFlagsGiven(args) && config.output !== 'json') {
process.stderr.write('--pr-reviews / --no-pr-reviews only apply to --type github; ignored.\n');
}
if (type === 'github') {
let prReviews = prReviewsChoiceFromFlags(args, canWaitForBrowser(config));
if (prReviews === 'ask') {
const answer = await askPrReviews(config);
if (answer === BACK) return BACK;
prReviews = answer;
}
url = githubConnectUrl(config, workspaceId, prReviews);
}
await openOrPrintInstallUrl(config, url, labels[type], noBrowser);

6. Flags and help

Both flags are scoped to GitHub in their descriptions, and the example list gains the opt-out form.

{ flag: '--no-browser', description: 'GitHub / Slack / Sentry / MCP OAuth: print the URL instead of opening it', type: 'boolean' },
{ flag: '--reconnect', description: 'GitHub / Slack / Sentry: run the connect flow even when the integration is already connected', type: 'boolean' },
{
flag: '--pr-reviews',
description: 'GitHub: review pull requests for production impact on the repositories this connection brings in (the default), without the prompt',
type: 'boolean',
},
{
flag: '--no-pr-reviews',
description: 'GitHub: do not review pull requests on the repositories this connection brings in; each repository can be changed later in the console',
type: 'boolean',
},
],
examples: [
'polylane integration connect',
'polylane integration connect --type github',
'polylane integration connect --type github --no-pr-reviews',
'polylane integration connect --type github --reconnect',

7. Tests

Pure-function tests for the decision table, the flag guard, the URL shape, and the [Y/n] parser.

https://github.com/coreplanelabs/cli/blob/04b8c18e6329867751cf4cfe0afdbdfb43689981/test/prompt-yes-no.test.ts#L5-L24

8. Remaining changes

  • test/integration-connect-pr-reviews.test.ts — decision table, flag guard, URL.
  • skill/SKILL.md — one paragraph telling agents about the question and the two flags (regenerated into src/generated/skill.ts by codegen).

Decisions

  • Ask in the CLI, not in install.sh. The installer is a thin shim over this CLI by design (AGENTS.md), and it already runs this command interactively; asking here means the question ships with a CLI release, needs no website deploy, and adds no shell-side version probe.
  • Installer [Y/n] shape, not a clack confirm. The first UAT run showed a boxed note and a clack Yes/No wedged between two [Y/n] lines; inside the installer the question must read like its neighbours, so it now uses the same prompt grammar and the same answer rule as ask_yn.
  • --pr-reviews exists alongside --no-pr-reviews. A non-interactive run that wants to record "yes, asked" needs a way to say so; without the positive flag the server could not tell "kept on" from "never asked".
  • Warn, do not error, when the flags meet a non-GitHub type (review F1). The type can be chosen from the picker after the flag was typed, so a usage error would fire after the user already answered.

Validation

  • --no-pr-reviews yields off regardless of interactivity — integration-connect-pr-reviews.test.ts
  • --pr-reviews yields on regardless of interactivity
  • No flag, interactive yields ask; no flag, non-interactive yields undefined
  • Both flags: opt-out wins
  • prReviewsFlagsGiven is true for either flag and false otherwise (the non-GitHub warning's guard)
  • URL carries pr_reviews=off / pr_reviews=on when answered and omits the parameter when not asked
  • parseYesNo: Enter keeps the default, n… no, y… yes, anything else keeps the default (mirrors ask_yn, verified against the live polylane.com/install.sh) — prompt-yes-no.test.ts
  • The connect test fails with the implementation reverted (verified: SyntaxError: ... does not provide an export named 'githubConnectUrl') and passes with it
  • npm test (full suite), npm run typecheck, npm run lint all green
  • Live on UAT through the real installer with this branch's bundle (previous head, clack prompt): nominal#2089 rows E1/E2/F1/F2, runs 260904001733e7vy and 260904001941emf0
  • Human-gated: one more lab run with the rebuilt bundle to see the [Y/n] line in the installer transcript (same commands as the ledger).

@coreplane-switchboard coreplane-switchboard Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM: Clean, well-tested CLI surface for the connect-time PR-reviews question; decision table, prompt gating, and URL building all check out.

  • [nit] F1 src/commands/integration/connect.ts:116 — --pr-reviews/--no-pr-reviews silently ignored for non-GitHub types

Verdict: approve — small, well-factored change; no correctness issues found. Reviewed head b5583ee.

What I checked:

  • Decision table (prReviewsChoiceFromFlags, connect.ts:110-119): opt-out beats opt-in when both flags are passed; interactive-without-flag → 'ask'; non-interactive-without-flag → undefined (nothing sent, server default). Matches the PR body's stated semantics and the tests pin all five cases.
  • Flag parsing: --no-pr-reviews is declared as its own boolean option, and the parser (src/args.ts kebabToCamel) maps it to noPrReviews: true — same pattern as the existing --no-browser/noBrowser, so no hidden --no- negation semantics to trip over.
  • Prompt gating (connect.ts:1080-1089): the question fires only when canWaitForBrowser(config) is true, which excludes --dry-run, --output json, and non-TTY runs — so scripts and agents never block on the confirm. BACK from the prompt propagates out of connectType and restarts the type picker, consistent with the other steps. The already-connected short-circuit at 1069-1077 runs first, so an existing connection is never re-asked (only --reconnect re-enters, which is reasonable).
  • URL building (githubConnectUrl): appends &pr_reviews=on|off to cliConnectUrl, which always already carries ?flow=...&workspace=..., so the bare & concat is safe and the values need no encoding. Omitted entirely when undefined, as the tests verify.
  • Verified locally: full npm test (449 pass, 0 fail), npm run typecheck, npm run lint all green on the PR head. (An initial test run right after npm ci showed transient failures that did not reproduce; two subsequent runs were fully green, and main is green too.)

Finding:

  • F1 (nit, connect.ts:116)--pr-reviews / --no-pr-reviews passed with a non-GitHub --type (e.g. --type slack --no-pr-reviews) are silently ignored rather than rejected. The help text scopes them to GitHub and other type-scoped flags behave the same way, so this is consistent with the existing convention — just noting a warning would be friendlier. Not blocking.

The SKILL.md paragraph and help/examples accurately describe the behavior. The one unchecked validation item (UAT round-trip against the nominal server PR) is correctly flagged as human-gated and doesn't affect this CLI-side review.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Auto-approved: coreplane-switchboard[bot] reviewed this PR and posted an LGTM verdict (see its review). A repo admin enabled this via the auto-approve workflow.

@justinhelmer
justinhelmer force-pushed the feat/github-connect-pr-reviews branch from b5583ee to 380e035 Compare September 3, 2026 22:39

@coreplane-switchboard coreplane-switchboard Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM: Re-review at rebased head 380e035: F1 (silent ignore of PR-review flags on non-GitHub types) is fixed with a stderr warning, JSON-safe guard, and test; full diff re-read, all checks green.

Verdict: approve — F1 addressed at head 380e035; no findings remain.

Re-review notes:

F1 (fixed, verified)connect.ts:1087-1089 now warns on stderr when --pr-reviews / --no-pr-reviews is passed with a non-GitHub type: --pr-reviews / --no-pr-reviews only apply to --type github; ignored. The prReviewsFlagsGiven guard is correct (true for either flag, false otherwise — pinned by the new test, including a noBrowser non-match case), and the warning is suppressed under --output json and written to stderr, so scripted/JSON consumers are unaffected. Placement inside connectType means it also fires when the type was picked interactively, which matches the comment's rationale.
• The branch was rebased into a single commit, so the prior head b5583ee no longer exists; the full diff vs main is 128 lines and I re-read it in whole. Nothing else changed in substance — decision table, prompt gating, URL building, and help text are as previously reviewed, plus the new SKILL.md paragraph accurately describes the flag behavior.
Verified locally: npm test (450 pass, 0 fail), npm run typecheck, npm run lint all green at the PR head. As in round 1, the very first test run right after npm ci showed transient failures (18 this time) that vanished on re-run — pre-existing flake in the harness warm-up, unrelated to this PR, but worth a look someday.

Clean fix round — nothing further.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Auto-approved: coreplane-switchboard[bot] reviewed this PR and posted an LGTM verdict (see its review). A repo admin enabled this via the auto-approve workflow.

…g GitHub

`integration connect --type github` asks one question before opening the browser: review pull requests for production impact on the repositories this connection brings in (default yes). The answer rides the /cli/connect URL as pr_reviews so the console records it on the integration before the repository sync, and the browser never asks a second time.

`--no-pr-reviews` opts out and `--pr-reviews` answers yes without the prompt. A non-interactive run without either flag sends nothing, so the server keeps its default and records no answer. The installer needs no change: it already runs this command interactively, so the question appears in the install flow as-is.
@justinhelmer
justinhelmer force-pushed the feat/github-connect-pr-reviews branch 2 times, most recently from 8068e20 to 04b8c18 Compare September 4, 2026 00:25

@coreplane-switchboard coreplane-switchboard Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM: Prompt reshape to the installer [Y/n] style is correct and well-tested; cancel/EOF map to BACK, no findings.

Verdict: approve — the prompt reshape at head 04b8c18 is correct; no findings. This round's verdict covers the whole PR, not just the delta.

The branch was rebased again (380e035 is gone), so I re-read the full 182-line diff vs main. The only substantive change since the last approval is the prompt mechanics:

promptYesNoOrBack / parseYesNo (prompt.ts:127-152) — clean readline implementation of the installer's [Y/n] shape. ensureInteractive still guards the non-TTY case; Enter keeps the default; parseYesNo faithfully mirrors ask_yn (first-char match, unrelated answers keep the default) and the new test pins all of that.
Cancel semantics verified — I checked directly against Node 22 (the repo's runtime): with no SIGINT listener on the interface, Ctrl+C closes readline, so the close handler resolves nullBACK, same as Ctrl+D/EOF. That matches the clack-confirm behavior it replaces — backing out returns to the type picker rather than hanging or hard-exiting. The double-resolve (question callback, then close in finally) is harmless since the promise is already settled.
askPrReviews (connect.ts:135-147) — the note box is replaced with a single plain stdout line before the question, which does read like install.sh's neighbouring Connect GitHub? [Y/n] lines. Only reachable in the interactive 'ask' path, so JSON/non-TTY runs are unaffected.
• Everything previously reviewed is unchanged in substance: the flag decision table, opt-out-wins precedence, non-GitHub warning (F1 fix from round 2, still present at connect.ts:1089-1091), URL building, help text, and the SKILL.md paragraph.

Verified locally: npm test 453 pass / 0 fail across two runs, npm run typecheck and npm run lint green. Third round in a row where the very first post-npm ci test run showed transient failures (18) that vanish on re-run — still pre-existing harness warm-up flake, unrelated to this PR, but it's now a pattern worth filing.

@justinhelmer
justinhelmer merged commit d4743ab into main Sep 4, 2026
3 checks passed
@justinhelmer
justinhelmer deleted the feat/github-connect-pr-reviews branch September 4, 2026 00:53

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Auto-approved: coreplane-switchboard[bot] reviewed this PR and posted an LGTM verdict (see its review). A repo admin enabled this via the auto-approve workflow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant